[16.0][FIX] rma_sale: Fix problems with link between RMA-sale.order and procurement.group-sale.order when creating from stock.picking.return wizard #415

TheClaud99 · 2024-07-31T16:07:04Z

Module

rma_sale

Describe the bug

There's some bugs in the rma_sale module when creating a return from the stock.picking.return wizard with the create_rma flag set.

To Reproduce

Affected versions:

Steps to reproduce the behavior:

Create a sale.order with a picking
Validate the picking
Create a return with the create_rma flag set.

Expected behavior
The order_id on the RMA and sale_id on the new procurement.group are set

…turn wizard

OCA-git-bot · 2024-07-31T16:07:08Z

Hi @chienandalu, @pedrobaeza,
some modules you are maintaining are being modified, check this out!

pedrobaeza · 2024-07-31T18:50:29Z

I don't get why you need this. Can you please explain the goal of having these fields set?

TheClaud99 · 2024-08-01T08:11:22Z

The field order_id is used for example as the inverse field in the rma_ids on sale.order, used for logic like the override of the function _get_invoiced. If you see my first commit 9787b8a there is a bug with a function name that causes it to never be called when it should set the order_id.

The field sale_id on the procurement.group is used for computing the field picking_ids on sale.order and in the actual version should be set by the function _prepare_procurement_group_vals in the module rma_sale but actually it never sets it because it checks for self.order_id but the function is called with no records in self.

pedrobaeza

OK, for the first commit. The second one seems weird to me and I don't get your explanation. If you see, the method is not covered by tests, so it seems still not called in usual flows. Other pending points:

You have to also check pre-commit CI, and the rest of the comments inline.

rma/wizard/stock_picking_return.py

rma_sale/wizard/stock_picking_return.py

TheClaud99 · 2024-08-01T10:19:42Z

Sorry for my bad explanation, I try to be more clear. When the method _prepare_procurement_group_vals is called from here

rma/rma/wizard/stock_picking_return.py

Line 83 in 3436ec6

vals = self.env["rma"]._prepare_procurement_group_vals()

the recordset in self is empty, so this condition will never be true because self.order_id will always be false:

rma/rma_sale/models/rma.py

Lines 167 to 171 in 3436ec6

    
           def _prepare_procurement_group_vals(self): 
        
               vals = super()._prepare_procurement_group_vals() 
        
               if not self.env.context.get("ignore_rma_sale_order") and self.order_id: 
        
                   vals["sale_id"] = self.order_id.id 
        
               return vals

and this override in this case doesn't work. My commit tries to fix this case, but if it's not ok I'll try to find a cleaner solution.

claudio-mano · 2024-08-05T12:48:41Z

Hi @pedrobaeza, I updated the code to be clearer, now the pre-commit CI works except for the tests. That's because the rma_sale module doesn't have a test method for the creation from stock.picking.return wizard like the one in the rma module, so the method I've added is not covered

rma/rma/tests/test_rma.py

Lines 679 to 708 in 3436ec6

    
           def test_rma_from_picking_return(self): 
        
               # Create a return from a delivery picking 
        
               origin_delivery = self._create_delivery() 
        
               stock_return_picking_form = Form( 
        
                   self.env["stock.return.picking"].with_context( 
        
                       active_ids=origin_delivery.ids, 
        
                       active_id=origin_delivery.id, 
        
                       active_model="stock.picking", 
        
                   ) 
        
               ) 
        
               stock_return_picking_form.create_rma = True 
        
               return_wizard = stock_return_picking_form.save() 
        
               picking_action = return_wizard.create_returns() 
        
               # Each origin move is linked to a different RMA 
        
               origin_moves = origin_delivery.move_ids 
        
               self.assertTrue(origin_moves[0].rma_ids) 
        
               self.assertTrue(origin_moves[1].rma_ids) 
        
               rmas = origin_moves.rma_ids 
        
               self.assertEqual(rmas.mapped("state"), ["confirmed"] * 2) 
        
               # Each reception move is linked one of the generated RMAs 
        
               reception = self.env["stock.picking"].browse(picking_action["res_id"]) 
        
               reception_moves = reception.move_ids 
        
               self.assertTrue(reception_moves[0].rma_receiver_ids) 
        
               self.assertTrue(reception_moves[1].rma_receiver_ids) 
        
               self.assertEqual(reception_moves.rma_receiver_ids, rmas) 
        
               # Validate the reception picking to set rmas to 'received' state 
        
               reception_moves[0].quantity_done = reception_moves[0].product_uom_qty 
        
               reception_moves[1].quantity_done = reception_moves[1].product_uom_qty 
        
               reception.button_validate() 
        
               self.assertEqual(rmas.mapped("state"), ["received"] * 2)

pedrobaeza · 2024-08-06T06:40:12Z

Thanks @TheClaud99. Now the code and the problem is much clearer now. I think the main problem is this call with no RMA:

rma/rma/wizard/stock_picking_return.py

Line 83 in 3436ec6

vals = self.env["rma"]._prepare_procurement_group_vals()

which is broken from the beginning. No partner is assigned as well.

What about creating the RMA, and then creating the procurement group from the RMA, so the rest of the code is valid? Putting also a self.ensure_one() in the prepare_vals will ensure no incorrect call is done.

TheClaud99 · 2024-08-06T08:49:51Z

The problem is that multiple RMAs could be created from the same stock.picking.return, which, according to the current logic, all share the same procurement.group:

rma/rma/wizard/stock_picking_return.py

Lines 98 to 112 in 3436ec6

    
           def _prepare_rma_vals_list(self): 
        
               vals_list = [] 
        
               for return_picking in self: 
        
                   global_vals = return_picking._prepare_rma_vals() 
        
                   for line in return_picking.product_return_moves: 
        
                       if ( 
        
                           not line.move_id 
        
                           or float_compare(line.quantity, 0, line.product_id.uom_id.rounding) 
        
                           <= 0 
        
                       ): 
        
                           continue 
        
                       vals = deepcopy(global_vals) 
        
                       vals.update(line._prepare_rma_vals()) 
        
                       vals_list.append(vals) 
        
               return vals_list

rma/rma/wizard/stock_picking_return.py

Lines 130 to 131 in 3436ec6

    
           vals_list = self._prepare_rma_vals_list() 
        
           rmas = self.env["rma"].create(vals_list)

Maybe the best solution could be to create the group by calling the method on the first created RMA and then set that group also to the others?

pedrobaeza · 2024-08-12T07:38:06Z

Yes, I think the call should be done with a RMA in self. Then, we decide later when creating subsequent RMAs if the procurement group is the same and then just assign it from the previous create. If it's the same group for all unconditionally, then just assign it.

TheClaud99 · 2024-08-19T15:29:07Z

Hi @pedrobaeza, another solution I thought is to use the same logic as here (function _group_delivery_if_needed()):

rma/rma/models/rma.py

Lines 1121 to 1124 in 09dd76a

    
           proc_group = self.env["procurement.group"].create( 
        
               rmas._prepare_procurement_group_vals() 
        
           ) 
        
           rmas.write({"procurement_group_id": proc_group.id})

I just also added this line

rma/rma/wizard/stock_picking_return.py

Line 128 in 910e73a

proc_group_vals["name"] = self.picking_id.name

to maintain the same logic as before for the group name

rma/rma/wizard/stock_picking_return.py

Lines 101 to 104 in 09dd76a

    
           origin = self.picking_id.name 
        
           vals = self.env["rma"]._prepare_procurement_group_vals() 
        
           vals["partner_id"] = partner_shipping.id 
        
           vals["name"] = origin

rma/wizard/stock_picking_return.py

…ng from stock.picking return wizard

TheClaud99 · 2024-08-20T08:08:36Z

@pedrobaeza I also made another fix that may be added to this pr: eeb9f7f.
The field sale_line_id on the move can be useful for populating the move_ids field on sale.order.line here:
https://github.com/odoo/odoo/blob/98408d48eecd7416275fbfc8b30769d844dbacba/addons/sale_stock/models/sale_order_line.py#L17
which is used for example to compute the delivered quantity on the sale line.
Can I merge the commit in this branch or should I create a new pr?

pedrobaeza · 2024-08-20T08:11:53Z

This is done on purpose, as we don't want to affect the invoceability of the sales order. It's linked later.

TheClaud99 · 2024-08-20T08:57:18Z

Ok sorry, I didn't see that it's linked later

sbejaoui · 2024-08-29T17:45:24Z

rma/wizard/stock_picking_return.py

+            proc_group_vals = rmas[:1]._prepare_procurement_group_vals()
+            proc_group_vals["name"] = self.picking_id.name
+            proc_group = self.env["procurement.group"].create(proc_group_vals)
+            rmas.write({"procurement_group_id": proc_group.id})


Hi @TheClaud99 ,

IMO, the issue is due to the non-unified of procurement groups creation.

When the picking return wizard is used, one procurement group is created for all RMAs. However, this is not the case when the sales order return wizard is used or when RMAs are created manually.

#423 addresses this issue by unifying the procurement group creation at RMA confirmation, regardless of the RMA origin.

I propose you revert these changes and keep the fix of the method name in rma_sale,

what do you think?

Hi @sbejaoui, I think it should be ok in your way as well (with also the advantage of having procurement.group creation unified), I can completely revert my commit 5f73bf6 and wait for your pr to be merged

[FIX] rma_sale: order_id not set on RMA when creating from picking re…

9787b8a

…turn wizard

pedrobaeza requested changes Aug 1, 2024

View reviewed changes

rma/wizard/stock_picking_return.py Outdated Show resolved Hide resolved

rma/wizard/stock_picking_return.py Outdated Show resolved Hide resolved

rma_sale/wizard/stock_picking_return.py Outdated Show resolved Hide resolved

rma_sale/wizard/stock_picking_return.py Outdated Show resolved Hide resolved

TheClaud99 force-pushed the patch-1 branch 2 times, most recently from 9ed0523 to d85e072 Compare August 2, 2024 16:21

pedrobaeza added this to the 16.0 milestone Aug 6, 2024

TheClaud99 force-pushed the patch-1 branch from d85e072 to 910e73a Compare August 19, 2024 15:14

pedrobaeza reviewed Aug 20, 2024

View reviewed changes

rma/wizard/stock_picking_return.py Outdated Show resolved Hide resolved

[FIX] rma_sale: sale_id not assigned to procurement_group when creati…

5f73bf6

…ng from stock.picking return wizard

TheClaud99 force-pushed the patch-1 branch from 910e73a to 5f73bf6 Compare August 20, 2024 07:28

pedrobaeza approved these changes Aug 20, 2024

View reviewed changes

pedrobaeza mentioned this pull request Aug 29, 2024

RMA: group reception #422

Open

sbejaoui suggested changes Aug 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][FIX] rma_sale: Fix problems with link between RMA-sale.order and procurement.group-sale.order when creating from stock.picking.return wizard #415

[16.0][FIX] rma_sale: Fix problems with link between RMA-sale.order and procurement.group-sale.order when creating from stock.picking.return wizard #415

TheClaud99 commented Jul 31, 2024 •

edited

Loading

OCA-git-bot commented Jul 31, 2024

pedrobaeza commented Jul 31, 2024

TheClaud99 commented Aug 1, 2024 •

edited

Loading

pedrobaeza left a comment

TheClaud99 commented Aug 1, 2024

claudio-mano commented Aug 5, 2024 •

edited

Loading

pedrobaeza commented Aug 6, 2024

TheClaud99 commented Aug 6, 2024

pedrobaeza commented Aug 12, 2024

TheClaud99 commented Aug 19, 2024

TheClaud99 commented Aug 20, 2024

pedrobaeza commented Aug 20, 2024

TheClaud99 commented Aug 20, 2024

sbejaoui Aug 29, 2024

TheClaud99 Aug 30, 2024

[16.0][FIX] rma_sale: Fix problems with link between RMA-sale.order and procurement.group-sale.order when creating from stock.picking.return wizard #415

Are you sure you want to change the base?

[16.0][FIX] rma_sale: Fix problems with link between RMA-sale.order and procurement.group-sale.order when creating from stock.picking.return wizard #415

Conversation

TheClaud99 commented Jul 31, 2024 • edited Loading

Module

Describe the bug

To Reproduce

OCA-git-bot commented Jul 31, 2024

pedrobaeza commented Jul 31, 2024

TheClaud99 commented Aug 1, 2024 • edited Loading

pedrobaeza left a comment

Choose a reason for hiding this comment

TheClaud99 commented Aug 1, 2024

claudio-mano commented Aug 5, 2024 • edited Loading

pedrobaeza commented Aug 6, 2024

TheClaud99 commented Aug 6, 2024

pedrobaeza commented Aug 12, 2024

TheClaud99 commented Aug 19, 2024

TheClaud99 commented Aug 20, 2024

pedrobaeza commented Aug 20, 2024

TheClaud99 commented Aug 20, 2024

sbejaoui Aug 29, 2024

Choose a reason for hiding this comment

TheClaud99 Aug 30, 2024

Choose a reason for hiding this comment

TheClaud99 commented Jul 31, 2024 •

edited

Loading

TheClaud99 commented Aug 1, 2024 •

edited

Loading

claudio-mano commented Aug 5, 2024 •

edited

Loading